fix(compile): rewrite backslashes on the PIO no-compile-CWD fallback path - #912
Conversation
Implements Phase 1 of the roadmap in #618: fbuild sync reads platformio.ini, classifies every lib_deps entry per env, and writes a deterministic JSON platformio.lock next to the ini. All flags in the issue proposal land: -e <env>, --yes, --locked, --check, --dry-run, --upgrade, --upgrade-package. Multi-env prompt gates whole-project sync; --yes / -e / --check bypass it. # Scope Phase 1 is a full CLI + classification + lockfile-shape ship. Network resolution (GitHub ref -> SHA, PIO registry version -> archive URL, archive sha256) is deferred to Phase 2 per the issue's staged rollout. Every remote entry gets status: "unresolved" with the raw spec + extracted owner/name/ref captured, so Phase 2 will only add fields, never renegotiate the schema. Local sources (symlink://, file://, filesystem paths — absolute POSIX, relative ./ / ../, Windows drive letters) get status: "unlocked" and are recorded verbatim for auditability. # Files Module layout (kept as one focused directory tree, three files, no new crate — respects the monocrate rule): - crates/fbuild-cli/src/sync/mod.rs — orchestrator + SyncArgs + run_sync + prompt logic - crates/fbuild-cli/src/sync/source.rs — SourceType + LockStatus + ClassifiedDep + classify() - crates/fbuild-cli/src/sync/lockfile.rs — Lockfile struct + JSON I/O + LockDiff comparison + atomic write via fbuild_core::fs::write_atomic_sync (from #865) - crates/fbuild-cli/src/cli/sync_cmd.rs — thin CLI adapter - crates/fbuild-cli/src/cli/args.rs — new Commands::Sync variant + KNOWN_SUBCOMMANDS entry - crates/fbuild-cli/src/cli/dispatch.rs — wire the new command - crates/fbuild-cli/src/cli/mod.rs — register sync_cmd - crates/fbuild-cli/src/main.rs — register sync module - docs/sync.md — user-facing docs - docs/CLAUDE.md — index entry # Lockfile schema (v1) - envs sorted alphabetically (BTreeMap) - packages per env sorted by (name, source_type, raw) - generated_at trimmed to seconds (ISO-8601 UTC) - fields with a documented consumer only (no decorative registry metadata — per issue decision) - package records DUPLICATED under each env (per issue decision: auditability > disk size) - read() refuses to load an unrecognized version — schema evolution is versioned # TDD-first — 30+ unit tests written before implementation Written first, then wired up: - crates/fbuild-cli/src/sync/source.rs::tests — 18 tests covering every documented lib_deps shape: - Registry: bare name, name@ver, owner/name@ver, whitespace, raw preservation - GitHub: bare URL, .git suffix, #ref, case-insensitive host - Git+: URL, ref - HTTP archive: .zip, .tar.gz - Local: symlink://, file://, ./relative, ../relative, /posix-abs, C:\ backslash, D:/ forward-slash - phase1_lock_status returns Unlocked for locals, Unresolved for remotes - crates/fbuild-cli/src/sync/lockfile.rs::tests — 10 tests: - Shape smoke - Packages sorted by name - Envs sorted - Local dep -> Unlocked - Registry dep -> Unresolved in Phase 1 - JSON deterministic for same input (regardless of insertion order) - Read/write roundtrip - Version mismatch rejected - compare fresh/stale detection (new dep added, env added, version changed) - JSON ends with newline - crates/fbuild-cli/src/sync/mod.rs::tests — 11 tests: - Missing platformio.ini is error - Single env writes lockfile - --check on missing lock is failed - --check on fresh lock passes - --locked on stale lock returns LockedFailed - --dry-run writes nothing - -e <env> skips multi-env prompt - -e <bad-env> is error - Second run without changes is NoOp - exit_code matrix (Wrote/NoOp/CheckPassed/DryRun=0, CheckFailed=1, LockedFailed=2, UserCancelled=3, Error=4) - skip_multi_env_prompt matrix - ISO date formatter basic + time-of-day # Local verification - `soldr cargo check -p fbuild-cli --all-targets` — clean - `soldr cargo clippy -p fbuild-cli --all-targets -- -D warnings` — clean - `soldr cargo test -p fbuild-cli --no-run` — clean (test binaries build) - `soldr cargo fmt -p fbuild-cli` — no-op Note on test execution: my local environment (soldr silently swallows all stdout/stderr; direct-cargo hits Windows SDK linker env issues that this session has documented at length in #899) prevents me from running the test binary end-to-end today. Every test is pure enough that the compile-clean under -D warnings + the TDD-first authoring order gives strong confidence, and CI will exercise them on Linux. Closes #618
CI on macos/windows failed with clippy `-D dead-code`: - SyncArgs.upgrade + upgrade_package — Phase 2 fields, captured from the CLI now so the argv surface is stable but not yet consumed. - SyncOutcome::Wrote(PathBuf) — payload used by structured callers (tests + future --json subcommand). Clippy doesn't see the tests as consumers. Annotate with #[allow(dead_code)] at the field / enum level with a comment pointing at #618's Phase 2.
…pile-CWD fallback path Windows PIO builds under `.build/pio/<board>/` don't have a `.fbuild` component upstream of the output path, so `compile_cwd_from_output()` returns None and `compile_source()` fell through to the raw `to_string_lossy()` on the source/output paths. That leaves backslashes intact in argv, which GCC's internal spec-file pass interprets as escape characters: `-c src\sketch\AutoResearchNet.cpp` → cc1plus.exe receives `srcsketchAutoResearchNet.cpp` → "fatal error: srcsketchAutoResearchNet.cpp: No such file or directory". Symptomatically identical to the fix that landed in #875 / #885 for the compile-CWD arm; the fallback arm was missed. Mirror the same `replace('\', "/")` guard so both paths agree on Windows. Bump workspace version 2.3.15 → 2.3.16. Repro: `bash compile esp32dev --examples AutoResearch` from a FastLED checkout on Windows failed at the first .cpp compile with `srcsketchAutoResearchNet.cpp` / `srcsketchAutoResearchBle.cpp` etc. After this fix, the compile completes end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… (#3476) Lands the real-hardware `I2sPeripheralEsp32DevEsp` class that implements `II2sPeripheralEsp32Dev` against IDF v4's `driver/i2s.h` DMA + ISR machinery. Completes the peripheral surface that was scaffolded in #3473 (Stage 1). ## What Stage 2 delivers - `i2s_peripheral_esp32dev_esp.h` — declares the real-hw class paralleling the mock. Same interface, injectable through the engine's ctor exactly like the mock. - `i2s_peripheral_esp32dev_esp.cpp.hpp` — implementation using IDF v4's `i2s_driver_install()` (DMA + ISR under the hood) + `i2s_write()` for async submit + `heap_caps_malloc(MALLOC_CAP_DMA)` for DMA-capable buffer alloc. Completion callback fires synchronously after `i2s_write` (which returns once the buffer has landed in the driver's DMA queue) — Stage 3 replaces this with a real DMA-done ISR trampoline. ## Deliberately not linked yet The new `.cpp.hpp` is intentionally NOT included in the classic- ESP32 `_build.cpp.hpp` unity build. Its `#include "driver/i2s.h"` pulls in IDF's `driver_ng` framework which conflicts at boot with the legacy register access in `i2s_esp32dev.cpp.hpp` (Yves Bazin's long-standing driver), producing: E (586) ADC: CONFLICT! driver_ng is not allowed to be used with the legacy driver abort() was called at PC 0x40154717 on core 0 → boot loop. Wiring the real-hw impl requires deleting the Yves TU first — that's Stage 3 scope. In the meantime, this PR delivers the type declaration (so future PRs can `#include` the header and construct the class) plus the fully-formed .cpp.hpp waiting to be linked once the Yves cleanup lands. ## Verification - `bash compile esp32dev --examples AutoResearch` — succeeds (fbuild fix for the PIO source-path backslash regression landed separately in FastLED/fbuild#912; without that fix even master was blocked on this box). - Flashed the resulting firmware to a WROOM on COM11; device boots cleanly (no ADC conflict, no core-dump abort), emits the standard `ready` RPC event with `pinRx:19, pinTx:18, drivers:5`, responds to `testGpioConnection([[33, 34]])` with `connected: true` (matching pre-#3473 behaviour bit-for-bit). - `bash test --cpp` — host suite **344/344** pass (Stage 1's 21 engine tests still green; new real-hw impl's presence is a no-op on host builds via the `#ifdef FL_IS_ESP32` guard). - `clang-format --dry-run --Werror` clean on every touched file. ## Follow-up (Stage 3, tracked separately) - Delete `i2s_esp32dev.{h,cpp.hpp}` + `clockless_i2s_esp32.{h,cpp.hpp}` (Yves Bazin's TU) so the driver_ng conflict clears. - Include `i2s_peripheral_esp32dev_esp.cpp.hpp` in the `_build.cpp.hpp`. - Wire `BusTraits<Bus::FLEX_IO, 0>` on classic ESP32 to construct `ChannelEngineI2sEsp32Dev` with a fresh `I2sPeripheralEsp32DevEsp` peripheral. - Fold `drivers/i2s_spi/` (SPI-only Bus::FLEX_IO,0 today) into the merged engine per the parallel-IO code-review rule. - Replace the sync completion callback with an ISR trampoline. - Replace the byte-copy `packScratchBuffer()` with the parallel bit-transpose + wave8 slot expansion. - WROOM on-device WS2812B decode via `bash autoresearch --i2s`. Ref #3474. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The #618 sync module and #912's backslash-rewrite fix were combined into a single squash-merge (ecd80c7). Four clippy fixes I'd already shipped on the sync branch didn't ride along, so main's `Check macOS` run is red on `-D warnings`. Reapplying: - fbuild-serial/src/boards.rs — field-reassign after Default::default() in the 1200bps-touch upload-hint test (from #906 carryover). Use struct-literal syntax + ..Default::default(). - fbuild-cli/src/sync/source.rs — swap the closure form `.rsplit(|c| c == '/' || c == '\')` for the array-pattern form `.rsplit(['/', '\'])`. Same behavior, satisfies clippy::manual_pattern_char_comparison. - fbuild-cli/src/sync/mod.rs — fold the nested `if !prompt_multi_env(..)` into the outer condition (clippy::collapsible_if). Same behavior. - fbuild-cli/src/cli/sync_cmd.rs — add `#[allow(clippy::too_many_arguments)]` on run_sync_cmd; the 8 args mirror clap's parsed variant 1:1 and wrapping them in a struct here would just push the noise into the dispatch site. No behavior changes.
…#1372) * ci(dylint): add a Windows leg so Windows-only code is actually linted Closes #1359. `cargo check` only compiles the modules the target platform selects, so a ubuntu-only Dylint job never sees a single `#[cfg(windows)]` or `platform/windows/**` module. The lint suite is the enforcement arm for most of this repo's path invariants, and Windows is where the bugs those lints exist to prevent actually bite — #875, #885, #890 and #912 were all Windows path handling. The gate was pointed away from the platform that needs it. Not hypothetical. A full sweep on a Windows host against main reports: error: use fbuild_core::path::NormalizedPath::display_slash() instead of hand-rolled backslash-to-slash rewrite --> crates\fbuild-core\src\platform\windows\fs.rs:21:17 ## Fixing the violation, not excusing it `ban_manual_slash_normalize` allowlisted `fbuild-core/src/path.rs` as "the primitive itself". The platform-facade migration (#1306) then moved the actual `\` -> `/` rewrite into the per-OS `platform::fs::display_slash`, leaving `NormalizedPath::display_slash` a one-line delegation — `path.rs` no longer performs the transformation at all. So the entry moves rather than being added alongside: the Windows implementation is the definition site now, and `path.rs` is dropped from this lint's allowlist. Keeping it "just in case" is how the list drifted out of step with the code in the first place. `path.rs` keeps its entries on the *other* lints, which it still earns. Crate version bumped, per the convention in its own manifest: the allowlist is embedded in the `.so`, and a stale cached copy on the ubuntu leg would silently skip enforcing the removal. ## The workflow change A matrix with per-leg pinned `name`, not a derived one. `Dylint` is a required status check, and letting the matrix rename the ubuntu leg to `Dylint (ubuntu-latest)` would leave that required check permanently pending on every PR. The ubuntu leg keeps reporting as exactly `Dylint`; the Windows leg is additive as `Dylint (windows)`. `fail-fast: false`, so a Windows violation cannot mask a Linux one. `defaults.run.shell: bash` for Git Bash on the Windows runner — the steps use process substitution and POSIX `find`, which PowerShell cannot parse. And `CARGO_HOME` is now defaulted where it was assumed: unset, `export PATH="${CARGO_HOME}/bin:..."` would silently prepend a bare `/bin`. ## Verified on Windows, which is the new leg Every step of the job was run locally on a Windows host: - full `dylint --all -- --workspace --all-targets` sweep: **0 violations** - the observed-comparison that closes the job (`enforce_platform_boundary.py --dylint-observed`): passes, `rows=14; dylint_rows=14` — a per-OS row disagreement was the likeliest way this leg could have failed for reasons unrelated to violations - the fmt loop and the test loop over all **27** lint crates: clean - `check_dylint_allowlists.py`, `render_workflows.py --check`, and `check_workflow_concurrency.py`: pass ## Not included: a macOS leg Same one-line matrix addition, deliberately deferred. `platform/macos/**` has never been linted by anyone, so its first run is pure discovery — and I have no way to triage it, having no macOS host. Windows has a demonstrated violation and local evidence; macOS has neither yet. Each leg also costs a ~17-30 min job on every PR push, which is worth stating rather than discovering: if that per-PR cost is unwanted, the Windows leg is equally useful restricted to `push: main`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci(dylint): scope the lint crates' own tests to the ubuntu leg The new Windows leg went red on its first run, and the finding is worth recording because it is not what the leg exists to catch. The lints themselves are clean on Windows — the workspace sweep passes. What failed is the "Test Dylint libraries" step, which runs each lint crate's own compiletest ui fixtures: error: could not load library `.../target/dylint-tests/debug/ban_manual_slash_normalize@nightly-2026-04-16.dll`: LoadLibraryExW failed Dylint 6.0.1 looks for its test library under `<target>/debug` while soldr sets `CARGO_BUILD_TARGET`, so cargo writes to `<target>/<host>/debug`. Each lint's `fn ui` already clears that variable — that is what the comment in every one of them is about — but on the runner the load still fails. A dylint/soldr/compiletest interaction, not an fbuild defect. Rather than work around it, the step is scoped to ubuntu, and the scoping is principled rather than a dodge: those are the lint crates' *own* tests, asserting each lint fires on its fixture. That behavior is platform- independent and already covered once. What the Windows leg uniquely provides is compiling Windows-gated *workspace* source so the lints can see it — which is the entire point of #1359, and which passes. The same reasoning scopes the fmt loop and the three Python validators, none of which learn anything from a second OS. Side benefit: the Windows leg gets ~10 minutes shorter, which matters for a job that runs on every PR push. The Windows leg now runs exactly the steps verified locally on a Windows host: the full sweep (0 violations) and the observed-comparison (`rows=14; dylint_rows=14`). Refs #1359 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci(dylint): pin the workflow token to contents: read CodeRabbit: the workflow had no `permissions` block, so it inherited the repository/organization default, which can include write access. Every step in this job reads — checkout, a shallow `git fetch origin main` for the shrink-only allowlist diff, and the lint runs themselves. Nothing writes, so nothing should be able to. Worth noting this is not yet the repo-wide convention: 8 of 106 workflows set `permissions` today. Hardening the rest is a worthwhile sweep but belongs in its own change rather than riding along here. Refs #1359 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Problem
Windows PIO builds under
.build/pio/<board>/don't have a.fbuilddirectory upstream of the output path, socompile_cwd_from_output()returns None. That dropscompile_source()into the rawto_string_lossy()fallback for the source/output args (lines 660-664 ofcompiler.rsbefore this PR), leaving backslashes intact in argv.GCC's internal spec-file pass then interprets each
\as an escape:Symptomatically identical to the class of bug that #875 / #885 fixed for the compile-CWD arm — the fallback arm was just missed at the time because it wasn't a hot path for local dev.
Fix
Mirror the same
replace('\', "/")guard on the fallback arm so both shapes agree on Windows.Reproduction
Before:
After:
Verified on a Windows 10 box with fresh WROOM flash — device boots, RPC works,
testGpioConnection([[33,34]])returnsconnected: true.Version
Bump workspace 2.3.15 → 2.3.16.
🤖 Generated with Claude Code